-
Notifications
You must be signed in to change notification settings - Fork 333
Add LoongArch support for kpatch v2 #1427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
@georgejguo FYI, instead of opening a new PR, next time you can just rebase your existing branch and force push it, which will reuse the existing PR. |
joe-lawrence
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @georgejguo - this is a really quick scan of the v2 patchset, see the github comments inline. A few follow ups:
- Are there LoongArch specific objects files that are generated by the kernel build, but aren't really part of the kernel itself? See kpatch-build/kpatch-cc and the long list of fileglobs that the are skipped. For example, I'd expect to see
arch/loongarch/vdso/*in there. - Without hardware, this will be difficult for us to test and maintain. Are there any public LoongArch resources available for running integration tests?
- Do the integration tests run for your kernel? (Which one(s) btw?)
- Would you be willing to generate a set of unit test object files (see https://github.com/dynup/kpatch-unit-test-objs where we keep those for various arches. I can walk you through how to generate them.)
- Last but not least, @jpoimboe what are your thoughts on supporting arches (arm64 and now LoongArch) that require out-of-tree kernel patches? I was leaning towards some kind of disclaimer and a very careful check that the kernel matches one of these special downstream versions.
| break; | ||
|
|
||
| case LOONGARCH64: | ||
| /* to be done */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I m not sure here, so just commit "to be done". Actually it can pass my simple test case.
kpatch-build/create-diff-object.c
Outdated
| bool found = false; | ||
| unsigned int *insn = sym->sec->data->d_buf + sym->sym.st_value; | ||
|
|
||
| if (*insn == LOONGARCH_NOP && *(insn + 1) == LOONGARCH_NOP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not believe this is endian-safe, ie, if this were run on a BE system like s390x (hypothetical/future cross compilation case), then the bytes would not be in the expected order. See other arch cases or https://github.com/dynup/kpatch/pull/52863dace0200332dc2ed5d8edab1f017e63b368 for specific bugfix where insn is treated as a sequence of bytes and not an unsigned integer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the link(https://github.com/dynup/kpatch/pull/52863dace0200332dc2ed5d8edab1f017e63b368) is 404 error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that, try this link: 52863da for ("create-diff-object: fix endianness in kpatch_no_sibling_calls_ppc64le()")
| rela->sym = rela->sym->sec->secsym; | ||
| log_debug("section symbol: %s\n", rela->sym->name); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, which kernel source files are generating these symbols?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey, I will try to find it.
kpatch-build/create-diff-object.c
Outdated
| sym->has_func_profiling = 1; | ||
| break; | ||
| case LOONGARCH64: | ||
| struct section *sec; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Older gcc complains about this:
gcc -std=gnu11 -MMD -MP -I../kmod/patch -Iinsn -Wall -Wsign-compare -Wconversion -Wno-sign-conversion -g -Werror -c -o create-diff-object.o create-diff-object.c
create-diff-object.c: In function ‘kpatch_find_func_profiling_calls’:
create-diff-object.c:4106:4: error: a label can only be part of a statement and a declaration is not a statement
struct section *sec;
^~~~~~
if this new variable remains in v3, please wrap the case block in {} brackets to appease those compilers.
Okey, I will clear it out and try to find ones if it turns out to be ture.
I have tested the code in Fedora-38 with kernel 6.12 and gcc 14.1 for Loongarch, just a simple cases.
Yeap, I am willing to do that.
The kernel has supported liveptch for Loongarch in upstream. [edited by Joe to cleanup the markdown for github web rendering] |
Great, thanks for heads up. I must have missed it as it seems like the patchset to turn on the config for that arch was never copied to the live-patching@vger.kernel.org mailing list (actually having some problems finding much lmkl discussion / posts, too). Anyway |
Hmm, I don't know of a programmatic way off the top of my head, maybe @jpoimboe has a suggestion. That said, the vdso code should be excluded for sure.
Sort of. Using a gcc-14.2.0 cross compiler and the latest kernel tree, I couldn't find any .LBB symbol instances outside of the vdso object files... that's why I was curious to know if it is something that create-diff-object really needs to account for (if the pattern never occurs in kernel code). |
|
hi, this is Add LoongArch support for kpatch v3 The updated file is only kpatch-build/create-diff-object.c according to your advice, Joe |
|
Hi @georgejguo , I tried this branch's version of create-diff-object against object files created by a gcc-14.2.0 cross compiler and the examples/cmdline.patch, but encountered this error, "changed section .rela.orc_unwind_ip not selected for inclusion". Then I found this stale PR: #1312 where @ZhangHongchen1 mentioned that section with regard to arch-specific differences on LoongArch. I also noticed that he has a detailed LoongArch support branch in his fork that accounts for several additional things, like trampolines for short (local?) function calls, load immediate instructions, etc. It would be worth seeing if @ZhangHongchen1 intends to pick up or continue that work and then combine efforts with a single PR. |
|
hi, this is Add LoongArch support for kpatch v4 The updated file is only kpatch-build/create-diff-object.c I also added Co-developed-by: zhanghongchen zhanghongchen@loongson.cn. Sorry, I lost this commit while pushing ”Add LoongArch support for kpatch V3“ |
Sorry, I lost a commit to fix this while pushing ”Add LoongArch support for kpatch V3“. |
|
hi,Joe |
|
This PR has been open for 60 days with no activity and no assignee. It will be closed in 7 days unless a comment is added. |
|
This PR has been open for 60 days with no activity and no assignee. It will be closed in 7 days unless a comment is added. |
1eb2f02 to
79b4354
Compare
Add section alt_instr check support for LoongArch. Signed-off-by: George Guo <guodongtai@kylinos.cn>
|
hi Joe, the fail log below:
Could you explain the test environment setup and how to run the unit tests? |
|
Hi @georgejguo : The test/unit/objs is a submodule reference to https://github.com/dynup/kpatch-unit-test-objs. You should be able to initialize / update it locally with a git cmd like: If you are going to create your own unit tests, it may just be easier to debug by replacing that objs directory with a set of your own files (adjust the arches accordingly) rather than messing with git submodules for the time being. The arch object file directory contains file sets:
Each object file should be stripped with You may use https://github.com/dynup/kpatch-unit-test-objs/pull/54/files as a template. You can use some of the integration test object files, but anything that is arch-specific will be most interesting. |
This suggests that the branch broke the aarch64 kpatch code. From the test's object file, the expected offset is 8 bytes and not 0 as the error describes: Any LoongArch-specific assumption about the function symbol vs. section beginning should be wrapped up in an arch-specific case. |
Add initial support for LoongArch. Signed-off-by: George Guo <guodongtai@kylinos.cn>
Generate 2 NOPs right at the beginning of each function with -fpatchable-function-entry=2 in LoongArch. Here process this situation. Co-developed-by: zhanghongchen <zhanghongchen@loongson.cn> Signed-off-by: zhanghongchen <zhanghongchen@loongson.cn> Signed-off-by: George Guo <guodongtai@kylinos.cn>
Here fix error like: "tcp.o: symbol changed sections: .LBB7266. create-diff-object: unreconcilable difference". Due to LoongArch GCC generating local labels such as .LBB7266, it is difficult to compare the modified sections in the corresponding object files of the two files before and after the patch, so change them with sections symbols in rela section, and delete them in other sections. Co-developed-by: zhanghongchen <zhanghongchen@loongson.cn> Signed-off-by: zhanghongchen <zhanghongchen@loongson.cn> Signed-off-by: George Guo <guodongtai@kylinos.cn>
Fix error: "changed section .rela.orc_unwind_ip not selected for inclusion". This section is about arch-specific differences on LoongArch, which is generated by LoongArch gcc. Signed-off-by: George Guo <guodongtai@kylinos.cn>
Since kpatch now supports LoongArch basically, enable the build. Signed-off-by: George Guo <guodongtai@kylinos.cn>
Added conditional compilation to prevent 'R_LARCH_64' and 'EM_LOONGARCH' from being referenced on x86 and other non-LoongArch architectures. This ensures the code works across different architectures without errors. Signed-off-by: George Guo <guodongtai@kylinos.cn>
Add arch/loongarch/vdso to the list of object files that kpatch-cc won't add to its changed_objs list. Signed-off-by: George Guo <guodongtai@kylinos.cn>
e4770a6 to
1a99e6c
Compare
… kernel panic On LoongArch systems, livepatch modules containing references to EXTERNAL global variables trigger kernel panics when the core kernel is built with -mdirect-extern-access optimization. Root cause: The -mdirect-extern-access optimization replaces GOT-based external symbol access with direct addressing for improved performance. However, this breaks the kernel module loading mechanism which relies on GOT entries for proper relocation of EXTERNAL symbol references. Direct access to global variables from livepatch modules causes invalid memory accesses and kernel panics. Solution: For LoongArch kpatch builds, conditionally disable direct-extern-access by adding: - -mno-direct-extern-access for GCC builds - -fno-direct-access-external-data for Clang builds Signed-off-by: George Guo <guodongtai@kylinos.cn>
…it symbol references Add architecture-specific -fPIC compiler flag for LoongArch64 to prevent kernel panics when applying livepatches containing references to symbols defined in the same compilation unit. Root cause: In the kpatch workflow, when a function is livepatched, it's extracted from the original object file and compiled into a separate kernel module. When the patched function references symbols defined in the same compilation unit (like 'uts_sem' in kernel/sys.c), these references break if not compiled as position-independent code. On LoongArch64, without -fPIC, references to same-compilation-unit symbols use absolute addressing that assumes fixed memory locations. When the function is relocated into the livepatch module, these absolute addresses become invalid, causing kernel panics. Example failure case: - SYSCALL_DEFINE1(newuname) references the same-compilation-unit symbol 'uts_sem' - When kpatch extracts this function into a module, the reference to 'uts_sem' must be properly relocated - Without -fPIC, the absolute address reference causes invalid memory access and kernel panic Solution: Force -fPIC compilation for all LoongArch64 kpatch builds. This ensures that references to same-compilation-unit symbols use position-independent addressing, allowing proper relocation by the kernel module loader and preventing kernel panics in livepatch scenarios. Co-developed-by: Kexin Liu <liukexin@kylinos.cn> Singed-off-by: Kexin Liu <liukexin@kylinos.cn> Signed-off-by: George Guo <guodongtai@kylinos.cn>
hi joe,
This pr is based for PR #1415.